Log key when rate limit has been exceeded. #57
Conversation
|
The change looks good, nice work. Although, my only concern is that it's possible for the key to contain sensitive information. Caddy does have config options to redact certain fields, but its HTTP server, for example, redacts certain values by default (sensitive header fields for instance). I wonder if we should make this an opt-in thing. 🤔 |
popcorn
force-pushed
the
drago/log-key-when-rate-limit-exceeded
branch
2 times, most recently
from
8c23714 to
d884263
Compare
| @@ -82,10 +82,11 @@ This is an HTTP handler module, so it can be used wherever `http.handlers` modul | |||
| "window": "", | |||
| "max_events": 0 | |||
| }, | |||
| "storage": {}, | |||
There was a problem hiding this comment.
I think this is in the wrong place.
popcorn
force-pushed
the
drago/log-key-when-rate-limit-exceeded
branch
from
bebafce to
4595b8a
Compare
|
Good thinking @mholt. I updated the code, squashed it into a single commit and pushed. Also I run the tests with Let me know what you think. |
handler.go
Outdated
| if h.LogKey { | ||
| h.logger.Info("rate limit exceeded", | ||
| zap.String("zone", zoneName), | ||
| zap.String("key", key), | ||
| zap.Duration("wait", wait), | ||
| zap.String("remote_ip", remoteIP), | ||
| ) | ||
| } else { | ||
| h.logger.Info("rate limit exceeded", | ||
| zap.String("zone", zoneName), | ||
| zap.Duration("wait", wait), | ||
| zap.String("remote_ip", remoteIP), | ||
| ) | ||
| } |
There was a problem hiding this comment.
What you can do here instead to avoid duplication is logger := h.logger.With(...) and put all the basic fields in there. Then, do if h.LogKey { ... } where you can add the key like logger = h.With(zap.String("key")).
You could alternatively build a slice of zap fields instead of using logger.With().
There was a problem hiding this comment.
Didn't know about that. Should be fixed now. Let me know if there's anything else.
popcorn
force-pushed
the
drago/log-key-when-rate-limit-exceeded
branch
from
f0bb470 to
17db5bf
Compare
|
Cool! Do I need to do anything else in order for this to get merged? |
|
Nope. :) |
|
Amazing, my first code contribution to Caddy 🤠 Cheers! |
|
Congrats! |
Why?
I'd like to know the key for which the rate limit has been exceeded.
Test
I tested both for
distributed: {}and without it.Caddy JSON rate limit setup
Resulting logs (prettified)
{ "level": "info", "ts": "2024-06-28T12:54:58.067Z", "logger": "http.handlers.rate_limit", "msg": "rate limit exceeded", "zone": "dynamic_example", "key": "localhost", "wait": 297.70859879, "remote_ip": "172.17.0.1" }I'm not an expert in Go so please let me know if this is good enough to merge.
Cheers!